Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Auditbeat] Add user information to processes #9963

Merged
merged 14 commits into from
Jan 29, 2019

Conversation

cwurm
Copy link
Contributor

@cwurm cwurm commented Jan 9, 2019

Since go-sysinfo can now report the UIDs and GIDs of a process, this adds this information to the process metricset.

The added fields are:

  • user.id (UID or SID)
  • user.name
  • user.group.id (GID or SID of primary group)
  • user.group.name
  • user.effective.id (EUID)
  • user.effective.group.id (EGID)
  • user.saved.id (SUID)
  • user.saved.group.id (SGID)

Also adds some unit tests and tightens the system test.

@cwurm cwurm added review needs_backport PR is waiting to be backported to other branches. Auditbeat SecOps labels Jan 9, 2019
@cwurm cwurm requested review from a team as code owners January 9, 2019 14:55
@elasticmachine
Copy link
Collaborator

Pinging @elastic/secops

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor nit that I'm ok if we fix subsequently (you decide).

Otherwise LGTM.

Thanks for the ping, btw, I created elastic/ecs#299

},
"user": {
"id": "1002",
"name": "elastic"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that ECS now defines the group field set as well. So you should also copy system.audit.process.gid to group.id.

Other than that, this event looks good

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, I've added group.id and group.name. I'm less comfortable having two duplicated fields (probably shouldn't even had one in the first place since we don't have those in other places either), so I've removed system.audit.process.uid and system.audit.process.gid.

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this works for me. LGTM

@andrewkroh
Copy link
Member

Auditbeat's auditd module already defines fields like user.sgid. It would be better to reuse the existing fields this way users can query and get all data that has the given user ID value. You'll need move fields from auditd up to the field.common.yml.

@cwurm
Copy link
Contributor Author

cwurm commented Jan 16, 2019

@andrewkroh You mean extend the top-level user field, right? I don't know about that - to me it feels that euid/suid/egid/sgid are more properties of the process entity, not the user (technically, they're also different users/accounts).

I took a look at the auditd module's sample data - it does not look ECS compliant to me? Fields like user.name_map and user.uid (instead of user.id) are not in ECS, and are not correctly documented in the fields.yml either (they're documented under the auditd.user namespace, but are actually in the top-level user object). That should not be I think? In Metricbeat and the Auditbeat system module, the system tests call Libbeat's assert_fields_are_documented(evt) to make sure that does not happen. Maybe this is something we should do for auditd as well?

@cwurm
Copy link
Contributor Author

cwurm commented Jan 16, 2019

I don't know about that - to me it feels that euid/suid/egid/sgid are more properties of the process entity, not the user (technically, they're also different users/accounts).

I could see how they would be part of the user object if that one is treated more like "any user information" rather than "the user entity". I wonder what it should look like then? The challenge is that at the moment ECS assumes the user fields to represent a single user, with one id, one name, one email, etc. And now we have multiple values.

If we follow Auditd, it would probably be:

"user": {
  "id": "1000",
  "name": "elastic",
  "euid": "1000",
  "suid": "1000",
  "name_map": {
    "euid": "elastic",
    "suid": "elastic"
  }

But I wonder, what if we ever wanted add more information than the name? Would we need more maps?

Maybe it could allow nesting, e.g.:

"user": {
  "id": "1000",
  "name": "elastic",
  "email": "elastic@localhost",    // example additional field
  "created": "2019-01-01 19:37:56" // example additional field
  "effective": { // Nested user object
    "id": "1000",
    "name": "elastic",
    "email": "elastic@localhost",    // example additional field
    "created": "2019-01-01 19:37:56" // example additional field
  },
  "saved": { // Nested user object
    "id": "1000",
    "name": "elastic",
    "email": "elastic@localhost",    // example additional field
    "created": "2019-01-01 19:37:56" // example additional field
  }
}

The group object is also affected, but would probably just follow whatever we do for user.

Anyway, if this is a change to the ECS user object, maybe this discussion has to happen in the ECS repo?

@webmat
Copy link
Contributor

webmat commented Jan 16, 2019

Yeah I addressed these fields under user in my review of Auditbeat/auditd here: #10111.

Waiting on input from Nic on it before bugging you guys. But my conclusion over in that issue is that despite not being in ECS (yet?), I don't think it's a problem. Read up over there for my thinking on this.

@cwurm
Copy link
Contributor Author

cwurm commented Jan 17, 2019

@webmat I see that in #10111 we have the group IDs under user as well: user.gid, user.egid, etc.

Whereas here we concluded to have them under group.

I'm confused now - I think we should have one or the other, and ECS should specify which.

Personally, I like the idea of having all user information under user and not have a top-level group object at all.

@webmat
Copy link
Contributor

webmat commented Jan 17, 2019

To follow up on the discussion we've had elsewhere, I would also recommend populating group.id with user.egid. This will help correlation with other places that are using group.id this way.

I agree that ECS will need to:

  • define how to represent user privilege escalation
  • clarify or review whether a user's group ID, when seen on an event belongs at group.id or nested under the user. This work will be related to the privilege escalation work. Just like with user IDs, multiple group IDs may have to be considered before determining the effective group ID

@cwurm I also think you mentioned that the sets of IDs represented here for privilege escalation is different than what has been happening in the new system module. I agree we would benefit from harmonizing this. However since this isn't in ECS yet and we can't tackle this before FF, I would say it's a decision that is up to you and Andrew on whether to move the fields around on one of the modules to align with the other.

Although you made the point that these details may make more sense under process. than under user., which also makes sense. Perhaps we touch neither of them for now?

@webmat
Copy link
Contributor

webmat commented Jan 17, 2019

Oops that first paragraph was meant as a recommendation for #10111. You're already doing this here :-) I have too many PRs to review LOL

@webmat
Copy link
Contributor

webmat commented Jan 17, 2019

Actually this whole comment was written for #10111 :-)

@cwurm
Copy link
Contributor Author

cwurm commented Jan 21, 2019

I've updated to the nested user format proposed in #10111 (comment) and rebased on master.

It would be nice to have resolved user and group names for effective and saved UIDs and GIDs (user.effective.name, user.effective.group.name, user.saved.name, user.saved.group.name), but I didn't want to do it here without a cache in place for those lookups. Something for a follow-up though.

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loving this representation of all the considered users, in determining the effective rights.

Let's wait for people's input tomorrow, when the US isn't on holiday. But I think this is good to go :-)

LGTM

"id": "1000"
},
"id": "1000"
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Can you please update the PR description to reflect the final format used in the PR.

@webmat
Copy link
Contributor

webmat commented Jan 23, 2019

Agree with @andrewkroh.

However this PR depends on #10275

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but a few points:

  • A rebase will get you green again
  • Please adjust the PR description with the final outcome
  • I think we're fine like this, wrt field definitions. ECS change was only adding user.group.id and user.group.name. You're defining everything you need here already. I'm defining them locally to Filebeat for the Fb auditd module.

@cwurm
Copy link
Contributor Author

cwurm commented Jan 28, 2019

jenkins, test this

1 similar comment
@cwurm
Copy link
Contributor Author

cwurm commented Jan 28, 2019

jenkins, test this

@cwurm cwurm merged commit fa40a54 into elastic:master Jan 29, 2019
@cwurm cwurm deleted the process_user_info branch January 29, 2019 12:33
cwurm pushed a commit to cwurm/beats that referenced this pull request Jan 29, 2019
Adds real, effective, and saved UID and GID information to the process dataset.

(cherry picked from commit fa40a54)
@cwurm cwurm added v6.7.0 and removed needs_backport PR is waiting to be backported to other branches. labels Jan 29, 2019
cwurm pushed a commit that referenced this pull request Jan 31, 2019
…es (#10395)

Adds real, effective, and saved UID and GID information to the process dataset.

(cherry picked from commit fa40a54, then adjusted)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants